Fix archive board action causing ~30s browser freeze#578
Conversation
Navigate to /boards before calling boardStore.deleteBoard so the BoardView is unmounted and its reactive subscriptions (sortedColumns, cardsByColumn, filter computeds) are torn down before the sequential state mutations fire. This eliminates the ~30-second browser freeze caused by cascading re-renders while the view was still mounted. Add loading state to the lifecycle action button to provide immediate feedback and prevent double-clicks. Fixes #519
Reorder state mutations in deleteBoard so detail refs (currentBoard, cards, labels, comments, presence) are cleared before the boards array is filtered. This prevents downstream watchers on `boards` from reading stale detail state during the reactive flush.
Verify that router.push fires before boardStore.deleteBoard to prevent reactive cascade freeze. Add tests for the disabled/loading button label during the archive action.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Self-Review FindingsRoot cause confirmedThe freeze was caused by sequential reactive mutations in Potential issues reviewed
VerdictNo blocking issues found. The fix is minimal and targeted at the root cause. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a performance issue (#519) where sequential reactive mutations caused a significant freeze during board archival. The fix involves reordering operations to navigate away from the board view before clearing state and updating the store to prevent cascading reactive updates. Feedback suggests using a finally block for state resets, correcting misleading comments regarding state assignments, and cleaning up redundant or confusingly named variables in the new test cases.
| } catch (error) { | ||
| console.error('Failed to update board lifecycle state:', error) | ||
| lifecycleActionInProgress.value = false | ||
| } |
There was a problem hiding this comment.
To ensure the lifecycleActionInProgress state is always reset, regardless of success or failure, it's better to use a finally block. This makes the component more robust, for instance if its visibility were controlled by v-show instead of v-if, where the component state is preserved.
} catch (error) {
console.error('Failed to update board lifecycle state:', error)
} finally {
lifecycleActionInProgress.value = false
}
| // Clear current board state in a single assignment to avoid cascading | ||
| // reactive updates. Previously, each `.value = …` triggered its own | ||
| // flush cycle, which caused watchers/computed properties in mounted | ||
| // views (BoardView, BoardCanvas, etc.) to re-evaluate on every | ||
| // intermediate state — the root cause of the ~30 s freeze in #519. |
There was a problem hiding this comment.
This comment is slightly misleading. It states "Clear current board state in a single assignment", but the code performs multiple assignments to different reactive refs. The reordering of operations is the key change here to ensure state consistency for any watchers. The comment should be updated to reflect this more accurately.
| // Clear current board state in a single assignment to avoid cascading | |
| // reactive updates. Previously, each `.value = …` triggered its own | |
| // flush cycle, which caused watchers/computed properties in mounted | |
| // views (BoardView, BoardCanvas, etc.) to re-evaluate on every | |
| // intermediate state — the root cause of the ~30 s freeze in #519. | |
| // Clear detailed state for the current board before removing it from the | |
| // main boards list. This prevents any watchers on the `boards` array | |
| // from accidentally accessing stale detail state (like cards, labels, etc.) | |
| // that belongs to the board being deleted. The primary performance fix for #519 | |
| // is unmounting the BoardView before this action is called. |
| it('should show loading label while archive action is in progress', async () => { | ||
| const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true) | ||
| let resolveDelete: () => void | ||
| mockStore.deleteBoard.mockReturnValue(new Promise<void>((resolve) => { resolveDelete = resolve })) | ||
|
|
||
| const wrapper = mount(BoardSettingsModal, { | ||
| props: { | ||
| board, | ||
| isOpen: true, | ||
| }, | ||
| }) | ||
|
|
||
| const archiveButton = wrapper | ||
| .findAll('button') | ||
| .find((btn) => btn.text().includes('Move to Archive')) | ||
| // Trigger without await so we can inspect intermediate state | ||
| void archiveButton?.trigger('click') | ||
| await wrapper.vm.$nextTick() | ||
| await wrapper.vm.$nextTick() | ||
|
|
||
| // The close event fires immediately, so the modal will already be hidden, | ||
| // but the button label should have been set to the in-progress state | ||
| expect(wrapper.emitted('close')).toBeTruthy() | ||
|
|
||
| resolveDelete!() | ||
| confirmSpy.mockRestore() | ||
| }) |
There was a problem hiding this comment.
This test case, should show loading label while archive action is in progress, is redundant and incomplete. It doesn't contain any assertions to verify the loading label. The subsequent test, should disable lifecycle button while action is in progress, already covers this behavior by checking for the button with the "Archiving..." text and verifying its disabled state. To avoid redundancy and keep the test suite clean, consider removing this test case.
|
|
||
| it('should disable lifecycle button while action is in progress', async () => { | ||
| const confirmSpy = vi.spyOn(window, 'confirm').mockReturnValue(true) | ||
| let resolveDelete: () => void |
There was a problem hiding this comment.
The variable resolveDelete is used to resolve the promise from the router.push mock. This is confusing as the name implies it's related to deleteBoard. Renaming it to resolvePush on lines 278, 279, and 301 would make the test's intent clearer.
Adversarial Review -- PR #578CriticalNone found. Major1. On the archive path,
Fix: Use a 2. If Wait -- re-reading the store code: Downgrading to informational -- no action needed. Minor3. Test This test's name promises it checks the loading label, but the only assertion is The subsequent test ("should disable lifecycle button while action is in progress") does properly verify the "Archiving..." text and disabled state, making this test purely dead weight. Recommendation: Either add a real assertion (e.g., check that the button text is "Archiving...") or remove this test entirely. 4. Misleading variable name The variable Recommendation: Rename to 5. Comment in The comment says "Clear current board state in a single assignment" but the code still does 6 individual Recommendation: Update the comment as Gemini suggested -- describe the reordering rationale, not a false claim about batching. 6. No test for the restore (un-archive) path's error handling There is no test that verifies behavior when Nits7. The 8. Missing Overall AssessmentPass with fixes. The core approach (navigate before mutating store state) is sound and well-targeted at the root cause. The only actionable issue is Major #1 (missing |
Review Fixes Applied (6c6d018)Addressed the following findings from the adversarial review:
All 1165 tests pass. Typecheck and production build clean. |
Update two analysis docs (chat-to-proposal gap and manual testing findings) to reflect recent fixes and testing status. Key changes: add Last Updated and status notes; mark Tier 1 improvements shipped (intent classifier regex/stemming/negation fixes, substring ordering bug, PR #579), UX parse hints shipped (PR #582), unit/integration tests shipped (PR #580), and note PR range #578–#582. In manual testing findings mark OBS-2/OBS-3 resolved (PR #581) and BUG-M5 resolved (PR #578), update resolutions and remove duplicate checklist items. Minor editorial clarifications and test counts added.
1 participant
Summary
boardStore.deleteBoard()sequentially cleared 6+ reactive refs (currentBoard, cards, labels, comments, presence, editingCard) while the BoardView was still mounted. Each mutation triggered a separate reactive flush, causing computed properties (sortedColumns, cardsByColumn, filteredCardCount) to re-evaluate on every intermediate state — cascading into ~30 seconds of synchronous DOM work./boardsbefore callingdeleteBoard()so the BoardView is unmounted and its reactive subscriptions are torn down before the state mutations firedeleteBoardto clear detail refs before filtering the boards array, preventing watchers onboardsfrom reading stale detail stateTest plan